Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MultiDB]: install default db config file when installing libswsscommon if there is no such a file #321

Merged
merged 1 commit into from
Dec 6, 2019

Conversation

dzhangalibaba
Copy link
Contributor

  • distribute database_config.json into /var/run/redis/sonic-db/ when installing libswsscommon.deb in postinst script

Signed-off-by: Dong Zhang d.zhang@alibaba-inc.com

@dzhangalibaba
Copy link
Contributor Author

@qiluo-msft installing default db config file if there is no one in expected location.

@qiluo-msft
Copy link
Contributor

Thanks for implement the logic manually. I guess http://man7.org/linux/man-pages/man5/deb-conffiles.5.html is just for that, could you help explorer?


Refers to: debian/postinst:12 in ef01c58. [](commit_id = ef01c58, deletion_comment = False)

@dzhangalibaba
Copy link
Contributor Author

Thanks for implement the logic manually. I guess http://man7.org/linux/man-pages/man5/deb-conffiles.5.html is just for that, could you help explorer?

Refers to: debian/postinst:12 in ef01c58. [](commit_id = ef01c58, deletion_comment = False)

Hi Qi, conffiles works, but there is one concern. When we use conffiles, if the config file already exists, installing deb pkg will give a prompt as below. The user needs to decide what to do next.
Did somne search, looks there is no better to skip this prompt in deb pkg installing. If we use this way, we need to add options when issusing "sudo dpkg -i --force-confold libswsscommon_1.0.0_amd64.deb" with --force-confold option to skip the prompt. Which means we need to modify all the build script which is using "dpkg -i libswsscommon_1.0.0_amd64.deb". This seems not good to me, it introduce more changes and did not get any big benefits. It also force installing to not overwrite existing files, which may be not good in future when there is a need to overwrite. Using manually copy in postinst can avoid this problem and didn't introduce extra changes.

Configuration file '/var/run/redis/sonic-db/database_config.json'
 ==> File on system created by you or by a script.
 ==> File also in package provided by package maintainer.
   What would you like to do about it ?  Your options are:
    Y or I  : install the package maintainer's version
    N or O  : keep your currently-installed version
      D     : show the differences between the versions
      Z     : start a shell to examine the situation
 The default action is to keep your current version.
*** database_config.json (Y/I/N/O/D/Z) [default=N] ? Y

@qiluo-msft
Copy link
Contributor

Thanks @dzhangalibaba !

Could you please share your code about "conffiles works" in another PR? I would like to explorer further. To suppress dpkg prompt is the right thing to do in future.

@dzhangalibaba
Copy link
Contributor Author

Thanks @dzhangalibaba !

Could you please share your code about "conffiles works" in another PR? I would like to explorer further. To suppress dpkg prompt is the right thing to do in future.

FYI, #323

DST_FILE="database_config.json"
SRC_PATH="/usr/share/swss"

# if there is no $DST_FILE, it is needed to copy one default to $DST_FILE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if file exists we should copy this one one wih suffix ".new" to the same directory to indicate that there is new configuration, btw. i though that there is mechanism already in debian packages to do that, instead manually do that for each file, here is some nice explanation: https://raphaelhertzog.com/2010/09/21/debian-conffile-configuration-file-managed-by-dpkg/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can add suffix to save this conf, but we won’t use confines as explained under Qi’s comment, we don’t want prompt.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@kcudnik
Copy link
Contributor

kcudnik commented Dec 5, 2019

Just as a question, our future goal is to support multiple syncd's running across system, controlled by single OA agent, some requirements i posted in my long email:

  • each syncd must have global unique id (0..255 etc)
  • each syncd must have database assigned (redis db index)
  • each syncd must support multiple switches, and each switch must have defined HWINFO for it (SAI_SWITCH_ATTR_SWITCH_HARDWARE_INFO (and maybe also SAI_SWITCH_ATTR_SWITCH_PROFILE_ID) this information is needed to uniquely identify every switch instance globally

my question here is whether we want this information in this file or do we need other file? this switch/syncd info is something more than db config, but it strictly depends on database configuration which is defined in this file

note: different syncd instances can ha same database server used, but asic_db index must be different for each syncd instance

@dzhangalibaba
Copy link
Contributor Author

Just as a question, our future goal is to support multiple syncd's running across system, controlled by single OA agent, some requirements i posted in my long email:

  • each syncd must have global unique id (0..255 etc)
  • each syncd must have database assigned (redis db index)
  • each syncd must support multiple switches, and each switch must have defined HWINFO for it (SAI_SWITCH_ATTR_SWITCH_HARDWARE_INFO (and maybe also SAI_SWITCH_ATTR_SWITCH_PROFILE_ID) this information is needed to uniquely identify every switch instance globally

my question here is whether we want this information in this file or do we need other file? this switch/syncd info is something more than db config, but it strictly depends on database configuration which is defined in this file

note: different syncd instances can ha same database server used, but asic_db index must be different for each syncd instance

I think we need extend current conf file to support the case you mention. And then add the backend logic to parse conf file to finish the work what you case want

@kcudnik
Copy link
Contributor

kcudnik commented Dec 6, 2019

seems good to me

@kcudnik kcudnik merged commit 29a2cdf into sonic-net:master Dec 6, 2019
@dzhangalibaba dzhangalibaba deleted the p2_deb branch December 17, 2019 19:56
abdosi pushed a commit that referenced this pull request Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants